frontend/packages/console-app/src/actions/hooks/useJobActions.ts (1)

55-57: Same launcher-dep suppression pattern—please confirm stability.

If useConfigureCountModal() returns a new function over time, omitting it from deps can leave the action stale. This mirrors the earlier launcher-dep concern.

frontend/packages/console-app/src/actions/providers/build-config-provider.ts (1)

36-38: Launcher-dep suppression repeated here—confirm the hook’s launcher is stable.

Same dependency omission pattern as in other action hooks; please ensure the launcher identity is stable or the action can become stale.

Also applies to: 71-73

frontend/public/components/utils/webhooks.tsx (1)

90-115: The isLoaded dependency may cause an unnecessary double-fetch.

The effect sets isLoaded = true at line 111, and isLoaded is also listed in the dependency array. On initial mount:

1. isLoaded is false → effect runs → secrets fetched → setLoaded(true)
2. isLoaded changes to true → effect re-runs → secrets fetched again

Consider adding a guard or removing isLoaded from deps if it's only meant to track completion:

  useEffect(() => {
-   if (!canGetSecret) {
+   if (!canGetSecret || isLoaded) {
      return;
    }

Alternatively, if the intent is to allow re-fetching when secretNames change, isLoaded should be removed from the dependency array entirely—it's not a meaningful trigger.

---

On the launchModal suppression: I see this pattern across the PR for avoiding the max-depth error. For long-term maintainability, the useOverlay hook should ideally return a stable (memoized) function reference. If that's not feasible right now, a useRef wrapper is a cleaner alternative to eslint-disable:

const launchModalRef = useRef(launchModal);
launchModalRef.current = launchModal;
// then use launchModalRef.current inside the effect

This keeps the linter happy and documents the intent more explicitly than a suppression comment.

frontend/packages/console-shared/src/hooks/useWarningModal.tsx (1)

37-52: Consider documenting memoization expectations.

The dependency array at line 50 includes props, which is an object reference. If callers pass inline objects (e.g., useWarningModal({ title: 'Title' })), the callback will be recreated on every render. This is fine for most use cases, but callers needing a stable callback reference should memoize their props.

Consider adding a note in the JSDoc (around line 30-36) mentioning this behavior, especially since other parts of the codebase may rely on stable callback references for effect dependencies.

📝 Suggested documentation addition

 /**
  * useWarningModal is a hook that provides a way to launch a WarningModal.
  * Supports two usage patterns:
  * - Pass props at hook initialization time: useWarningModal({ title: 'Title' })
  * - Pass props in the callback: const launch = useWarningModal(); launch({ title: 'Title' })
  * - Mix both (overrides have priority): const launch = useWarningModal({ title: 'Title' }); launch({ children: <Content /> })
+ *
+ * Note: If a stable callback reference is required, memoize the props object passed
+ * to this hook using useMemo to prevent the callback from being recreated on each render.
  */

frontend/packages/knative-plugin/src/components/add/SecretKeySelector.tsx (1)

51-63: Confirm useOverlay() provides a stable function or capture it safely.
Line 51–63 suppresses exhaustive-deps and drops launchModal. If useOverlay() ever returns a new function (e.g., provider change), this effect will keep a stale launcher and may miss error modals. Please confirm stability; if not guaranteed, capture it via a ref while keeping the namespace-only fetch behavior.

♻️ Optional safer pattern (ref keeps latest launcher)

-import { useState, useEffect } from 'react';
+import { useState, useEffect, useRef } from 'react';
...
-  const launchModal = useOverlay();
+  const launchModal = useOverlay();
+  const launchModalRef = useRef(launchModal);
+  useEffect(() => {
+    launchModalRef.current = launchModal;
+  }, [launchModal]);
...
-        if (err?.response?.status !== 403) {
-          launchModal(ErrorModal, { error: err?.message });
-        }
+        if (err?.response?.status !== 403) {
+          launchModalRef.current(ErrorModal, { error: err?.message });
+        }

frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx (2)

42-42: Minor inconsistency in CTA assignment pattern

Line 34 assigns cta: startNodeMaintenanceModal directly as a function reference, whereas here you wrap with cta: () => stopNodeMaintenanceModalLauncher(). Both work, but the inconsistency is a bit puzzling.

If both launchers have the same signature (no-arg functions returning void/Promise), prefer consistency—either direct reference for both or arrow wrappers for both. This aids readability and avoids questions about subtle differences.

♻️ Suggested alignment (if signatures match)

-        cta: () => stopNodeMaintenanceModalLauncher(),
+        cta: stopNodeMaintenanceModalLauncher,

---

27-28: Guard the useStopNodeMaintenanceModal hook call or add undefined handling

findNodeMaintenance returns undefined when no matching maintenance is found, yet useStopNodeMaintenanceModal(nodeMaintenance) is invoked unconditionally at line 28. The hook immediately computes modal content by calling getNodeMaintenanceReason(nodeMaintenance) and getNodeMaintenanceNodeName(nodeMaintenance) with potentially undefined data. Additionally, getMaintenanceModel(nodeMaintenance) (invoked in the onConfirm callback) accesses nodeMaintenance.apiVersion without checking if it exists.

While the launcher is only invoked when nodeMaintenance is truthy (line 38), this pattern wastes renders and violates the hook's contract—the type signature doesn't reflect that undefined is acceptable. This can be fragile if the selector functions don't handle undefined gracefully.

Either guard the hook call with a conditional or update useStopNodeMaintenanceModal to explicitly handle and return a no-op launcher when nodeMaintenance is undefined.

frontend/packages/knative-plugin/src/actions/creators.ts (1)

130-130: Unnecessary template literal.

Use a plain string literal here since there's no interpolation.

✏️ Suggested fix

-      id: `delete-resource`,
+      id: 'delete-resource',

frontend/packages/console-app/src/actions/hooks/useRetryRolloutAction.ts (1)

98-100: Re-enable react-hooks/exhaustive-deps and include launchModal in the dependency array.

useOverlay returns a stable launcher—launchOverlay is memoized with useCallback(..., []) in OverlayProvider (line 40), so its identity never changes. The suppression is unnecessary; including it in the deps array will not cause infinite loops. Keeping the lint rule enabled ensures future maintainers catch unintended stale closures.

frontend/packages/console-app/src/actions/hooks/useCommonActions.ts (1)

47-85: Avoid suppressing exhaustive-deps; verify useOverlay stability.

launchModal is omitted from deps with an eslint-disable. If useOverlay ever returns a new function, this can capture a stale launcher. Prefer stabilizing the hook output (or memoizing) so the dependency can be included and the suppression removed.

Also applies to: 174-176

frontend/packages/console-app/src/actions/hooks/usePVCActions.ts (1)

7-11: Inconsistent lazy-loading for modal providers.

LazyExpandPVCModalProvider and LazyClonePVCModalProvider are lazy-loaded, but DeletePVCModalProvider is imported directly. For consistency and code-splitting benefits, consider using a lazy-loaded variant for DeletePVCModalProvider as well.

Does React lazy loading improve bundle size for modals?

frontend/public/components/modals/managed-resource-save-modal.tsx (1)

53-54: Consider excluding closeOverlay from prop spread.

The {...props} spread will pass closeOverlay to the inner ManagedResourceSaveModal, which doesn't use it (it uses close instead). While React ignores unknown props on function components, explicitly destructuring to exclude closeOverlay would be cleaner:

♻️ Suggested refinement

 export const ManagedResourceSaveModalProvider: OverlayComponent<ManagedResourceSaveModalProps> = (
   props,
 ) => {
+  const { closeOverlay, ...rest } = props;
   return (
-    <ModalWrapper blocking onClose={props.closeOverlay}>
-      <ManagedResourceSaveModal close={props.closeOverlay} {...props} />
+    <ModalWrapper blocking onClose={closeOverlay}>
+      <ManagedResourceSaveModal close={closeOverlay} {...rest} />
     </ModalWrapper>
   );
 };

frontend/public/components/namespace.jsx (1)

878-882: Simplify: pullSecret: undefined is redundant.

Passing pullSecret: undefined explicitly is unnecessary since the prop is already optional in ConfigureNamespacePullSecretProps. This adds noise without semantic value.

♻️ Suggested simplification

   const modal = () =>
     launchModal(LazyConfigureNamespacePullSecretModalProvider, {
       namespace,
-      pullSecret: undefined,
     });

frontend/packages/console-app/src/components/modals/resource-limits/index.ts (1)

4-9: The .then(m => ({ default: m.default })) is redundant.

Since ResourceLimitsModalLauncher already exports ResourceLimitsModalProvider as its default export, the transform is unnecessary. React.lazy expects a module with a default export, which is already satisfied.

♻️ Simplified lazy export

 export const LazyResourceLimitsModalProvider = lazy(() =>
-  import('./ResourceLimitsModalLauncher' /* webpackChunkName: "resource-limits-modal" */).then(
-    (m) => ({
-      default: m.default,
-    }),
-  ),
+  import('./ResourceLimitsModalLauncher' /* webpackChunkName: "resource-limits-modal" */),
 );

That said, if this pattern is intentional for consistency across the codebase or to handle potential named-export scenarios in the future, feel free to keep it.

frontend/packages/dev-console/src/actions/context-menu.ts (1)

14-40: Inconsistency: DeleteApplicationAction still uses i18next.t() directly.

This function uses i18next.t() for translations (line 23), while the new useDeleteResourceAction hook uses useTranslation(). Since DeleteApplicationAction is not a hook, it can't use useTranslation, but this creates an inconsistency in how translations are handled within the same file.

Consider migrating DeleteApplicationAction to a hook (useDeleteApplicationAction) in a future iteration to align with the hook-based pattern if the broader migration effort supports it.

frontend/packages/console-app/src/components/modals/resource-limits/ResourceLimitsModalLauncher.tsx (1)

64-70: Prop spread order could inadvertently override close.

The spread {...props} after close={props.closeOverlay} means if props somehow contains a close property, it would override the intended value. While OverlayComponent props shouldn't include close (they provide closeOverlay), defensive ordering is safer:

♻️ Defensive prop ordering

 export const ResourceLimitsModalProvider: OverlayComponent<ResourceLimitsModalLauncherProps> = (
   props,
 ) => (
   <ModalWrapper blocking onClose={props.closeOverlay}>
-    <ResourceLimitsModalLauncher close={props.closeOverlay} {...props} />
+    <ResourceLimitsModalLauncher {...props} close={props.closeOverlay} />
   </ModalWrapper>
 );

This ensures close is always bound to closeOverlay regardless of what's in props.

frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (1)

317-329: Same prop spread order concern; also closeOverlay leaks into child props.

Two observations:

1. Prop ordering: The spread {...props} after explicit cancel and close assignments could override them if props contains those keys.

2. closeOverlay prop leakage: The OverlayComponent type injects closeOverlay into props. Spreading {...props} passes closeOverlay to ConfigureNamespacePullSecret, which doesn't expect it. This may cause a React warning about unknown DOM attributes if the prop propagates further.

♻️ Destructure to exclude closeOverlay and fix ordering

-export const ConfigureNamespacePullSecretModalProvider: OverlayComponent<ConfigureNamespacePullSecretProps> = (
-  props,
-) => {
+export const ConfigureNamespacePullSecretModalProvider: OverlayComponent<ConfigureNamespacePullSecretProps> = ({
+  closeOverlay,
+  ...restProps
+}) => {
   return (
-    <ModalWrapper blocking onClose={props.closeOverlay}>
+    <ModalWrapper blocking onClose={closeOverlay}>
       <ConfigureNamespacePullSecret
-        cancel={props.closeOverlay}
-        close={props.closeOverlay}
-        {...props}
+        {...restProps}
+        cancel={closeOverlay}
+        close={closeOverlay}
       />
     </ModalWrapper>
   );
 };

This prevents closeOverlay from leaking and ensures cancel/close are always correctly bound.

frontend/public/components/modals/index.ts (1)

87-92: Minor formatting inconsistency.

LazyClonePVCModalProvider and LazyRestorePVCModalProvider use single-line .then((m) => ({ default: m.default })) while the other six lazy providers use multi-line formatting. Consider aligning for consistency and easier diffing across providers.

📝 Suggested formatting alignment

 // Lazy-loaded OverlayComponent for Clone PVC Modal
 export const LazyClonePVCModalProvider = lazy(() =>
   import(
     '@console/app/src/components/modals/clone/clone-pvc-modal' /* webpackChunkName: "clone-pvc-modal" */
-  ).then((m) => ({ default: m.default })),
+  ).then((m) => ({
+    default: m.default,
+  })),
 );

 // Lazy-loaded OverlayComponent for Restore PVC Modal
 export const LazyRestorePVCModalProvider = lazy(() =>
   import(
     '@console/app/src/components/modals/restore-pvc/restore-pvc-modal' /* webpackChunkName: "restore-pvc-modal" */
-  ).then((m) => ({ default: m.default })),
+  ).then((m) => ({
+    default: m.default,
+  })),
 );

Also applies to: 113-118